Add Health Checks #36
Conversation
…d external to build the links automatically.
There was a problem hiding this comment.
Just a few minor things, @bradp. Everything else looks good to me! 😃
| /** | ||
| * Add performance health checks. | ||
| */ | ||
| class HealthCheckManager { |
There was a problem hiding this comment.
Shall we move the health checks related files to a HealthChecks directory? It will help keep the related code better organized.
includes/HealthCheckManager.php
Outdated
| * | ||
| * @var Container | ||
| */ | ||
| protected $container; |
There was a problem hiding this comment.
Do we need $container? I don't see it being used anywhere in the class.
includes/HealthCheckManager.php
Outdated
| * | ||
| * @param array $options Health check options. | ||
| */ | ||
| public function addHealthCheck( $options ) { |
There was a problem hiding this comment.
Let's stick to using snake casing for function names, as per WordPress standards.
| */ | ||
| public function addHealthCheck( $options ) { | ||
| $options = wp_parse_args( | ||
| $options, |
There was a problem hiding this comment.
Let's add some documentation to explain what these keys represent and how the values affect the functionality
includes/HealthChecks.php
Outdated
| $manager->addHealthCheck( | ||
| array( | ||
| 'id' => 'autosave-interval', | ||
| 'title' => __( 'Autosave Interval', 'newfold-performance-module' ), |
There was a problem hiding this comment.
| 'title' => __( 'Autosave Interval', 'newfold-performance-module' ), | |
| 'title' => __( 'Autosave Interval', 'newfold-module-performance' ), |
I think this is the right namespace we're using for the i18n function.
includes/HealthChecks.php
Outdated
| /** | ||
| * Add health checks. | ||
| */ | ||
| public function addHealthChecks() { |
There was a problem hiding this comment.
I think this works well for now. But if this file grows into a large monolith in the future, do you think it might make sense to create an abstract class for new health checks to inherit from?
For instance, we could have a HealthChecks directory with an AbstractHealthCheck.php file that defines and documents all the necessary fields. A class like LinkPrefetchHealthCheck.php could then inherit from it and include all tests related to link prefetching.
In this file, we can simply call $manager->addHealthCheck( new LinkPrefetchHealthCheck() );. This approach would provide better validation and typing in PHP, prevent files from becoming too large, group related functionality, and avoid people adding random data directly into the manager. What do you think?
|
|
||
| /* // phpcs:ignore Squiz.PHP.CommentedOutCode | ||
| // Enable when https://github.com/newfold-labs/wp-module-performance/pull/26 is merged. | ||
| // PRESS7-120: Link Prefetching. |
There was a problem hiding this comment.
The Link Prefetch and Jetpack Boost stuff has been merged. I'm working on getting the Image Optimization changes merged as soon as possible after a review! 😃
Proposed changes
Implements a variety of Health Checks to the Site Health page.
Jira tickets: PRESS7-108, PRESS7-109, PRESS7-110, PRESS7-111, PRESS7-118, PRESS7-112, PRESS7-113, PRESS7-121, PRESS7-107, PRESS7-119, PRESS7-120, PRESS7-114, PRESS7-115, PRESS7-116, PRESS7-117.
Type of Change
Screenshot
Checklist
Further comments
This PR adds
HealthCheckManager.phpwhich is a helper class to set up and add health checks to the Site Health page. This is used inHealthChecks.phpto add the health checks to the Site Health page.This makes it so adding a new health check is as simple as:
This PR also adds Health Checks that are commented out for the following PRs